Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ISA test #29

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add ISA test #29

wants to merge 18 commits into from

Conversation

rmn30
Copy link
Collaborator

@rmn30 rmn30 commented Mar 10, 2023

This adds a test that attempts to trigger various ISA corner cases and increase test coverage.

@rmn30 rmn30 force-pushed the isa_test branch 2 times, most recently from f11cf92 to 44b3b0b Compare March 10, 2023 18:10
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@nwf-msr nwf-msr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, this is really great. The combinator approach works out really nicely, even if C++ will never be as nice as Haskell. ;)

A small pile of nits, but nothing big.

tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Show resolved Hide resolved
tests/isa-test.cc Show resolved Hide resolved
*/
void do_load_test(CauseCode expectedFault,
CapabilityFilter<int *> baseFilter,
size_t anExpectedMCause = MCauseCheri)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nit, but maybe the argument order should be baseFilter, expectedFault, anExpectedMCause, so that it's read as something like "I expect a load with this filter to cause this fault and this code"?

tests/isa-test.cc Outdated Show resolved Hide resolved
tests/isa-test.cc Show resolved Hide resolved
* nops so that we can test things like half of an instruction being in
* bounds. Clang doesn't seem to emit a prelude for this function. If it
* does (e.g. at -O0) our expected error PC calculations will be wrong due
* to alignment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these, too, should be in assembler?

@rmn30 rmn30 force-pushed the isa_test branch 2 times, most recently from 3642b13 to c5108c1 Compare July 1, 2024 15:29
// sentry {}", reinterpret_cast<void*>(interruptsDisabledSentry.get()));
// TEST(interruptsDisabledSentry.type() == 2,
// "Expected type 2 but got {}",
// interruptsDisabledSentry.type());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no good way to get these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the linker error in the comment. Suspect it might be a bug @davidchisnall ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmn30 rmn30 force-pushed the isa_test branch 2 times, most recently from 4bf1383 to 490ee6a Compare November 18, 2024 15:10
@rmn30 rmn30 force-pushed the isa_test branch 2 times, most recently from bc5a2a2 to 72f776e Compare December 10, 2024 18:16
rmn30 and others added 14 commits December 11, 2024 14:27
This specifically aims to test features that are not likely to occur
in normal operation, such as exceptions. Hits quite a few potential
corner cases but some are best tested in bare metal assembly and can't
be easily tested in an RTOS tests.
Co-authored-by: Nathaniel Filardo <[email protected]>
The static casts in the pointer DebugFormatArgumentAdapters prevented
function pointers from being printed as the compiler does not permit
static casts from function pointer to void*. Removing them seems to
work fine so not sure what they were there for.
Still some TODOs to resolve.
… review.

Also address a few other review comments.
behaviour

The previous approch no longer worked because the compiler would
choose a non-ra return register for the jalr, which means the return
register is no longer sealed, so remove the asm hack and use a helper
that uses __builtin_return_address. This new version is much cleaner
anyway.
It must return int so that test-runner can detect unintended faults / unwinds.
This allows us to continue even if we don't know where the faulting instruction is (for example after triggering a bounds fault on cjalr).
As Wes points out they do not necessarily fault so rename them to the
more neutral perform_load / perform_store.
It's good to check that memory wasn't modified in faulting cases and we stored the expected data in non-faulting cases.
One of the tests is redundant after an ISA change: it makes no difference whether the tag of a local capability being stored is set because it should will be cleared anyway.
Previously it would fault only if the tag was set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants